fix: budget the drain against the container's own healthcheck - #110
Conversation
bb1736a to
022601d
Compare
|
Four review findings, all real. Fixed and force-pushed as 1. A leftover raw echo defeated the normalisation in the same hunk
Every emitted duration now goes through the model. The test asserts both the normalisation and that no emitted duration carries a unit Compose cannot parse. 2. The
|
A rolling deploy marks a container unhealthy, waits for the runtime to notice, and only then stops it — so the proxy has stopped routing before the container goes away. The wait was budgeted as retries × the cadence Onebox polls `docker inspect` at, while the flip it waits for happens at retries × the interval the container's own healthcheck runs at. Those are different jobs: one is a local query, the other a probe inside the container. They agreed only when health.interval was authored. With the shorthand `health: /path` no interval was written into the generated healthcheck at all, so the runtime applied its own 30s default while Onebox budgeted against its 2s poll cadence: a flip needing 90s against a 10s budget, timing out on every replica of every deploy, and stopping each container while the proxy might still be routing to it — precisely what the budget exists to prevent. The budget now comes from the draining container itself, read back with the same inspect that already checks whether its check is drain-guarded. A healthcheck is baked in at creation, so the spec being deployed describes the containers being started, never the ones being drained. Budgeting from the spec would leave the failure intact for one more deploy after any change to the probe timing — including a change to Onebox's own default, which no operator asked for and which would otherwise strand every replica of the first deploy after an upgrade. Alongside that, the generated healthcheck stops leaving anything to the runtime's defaults. Interval, retries and start period are all written down, so what Onebox reasons about and what the container does cannot drift apart. An unset interval becomes 5s rather than the runtime's 30s: a probe every five seconds costs twelve requests a minute per container and lets a drained one leave rotation in fifteen, which keeps a rolling deploy from being dominated by waiting for the flip. Writing down a fast interval is only safe with a grace period to match. Before, the runtime's 30s interval gave a booting container roughly a minute and a half before a failed probe could count against it; a 5s interval with no start period would call a slow-starting application unhealthy while it is still coming up, and that verdict is visible to dependency conditions, restart watchers and alerting long before the rollout would notice. The start period is therefore written too, at 30s, which is the delay the first probe already had. The runtime leaves it at the first success, so a fast application pays none of it. The readiness budget stretches to cover one full flip cycle when the probe timing needs it. 120s is a figure chosen for ordinary timings, and with `interval: 3m` the newcomer's first probe lands after the budget expires — the rollout would remove a container that was never given the chance to answer. An authored `within` is still taken at its word. Retries gains an upper bound. It multiplies the probe interval to give the budget, and a count large enough to overflow that arithmetic yields a negative budget: one that expires immediately, which is the original failure reached by another route. Validating that retries × interval fits the budget is deliberately not added. The budget is (retries + 2) × interval against a flip of at most (retries + 1) × interval, so it covers the flip by construction. Every generated runtime that declares a healthcheck changes, because it now carries three keys it previously left to the runtime. Frozen corpus verdicts are regenerated accordingly. Closes #109
022601d to
fb9c2f8
Compare
A rolling deploy marks a container unhealthy, waits for the runtime to notice, and only then stops it — so the proxy has stopped routing before the container goes away. The wait was budgeted as retries × the cadence Onebox polls `docker inspect` at, while the flip it waits for happens at retries × the interval the container's own healthcheck runs at. Those are different jobs: one is a local query, the other a probe inside the container. They agreed only when health.interval was authored. With the shorthand `health: /path` no interval was written into the generated healthcheck at all, so the runtime applied its own 30s default while Onebox budgeted against its 2s poll cadence: a flip needing 90s against a 10s budget, timing out on every replica of every deploy, and stopping each container while the proxy might still be routing to it — precisely what the budget exists to prevent. The budget now comes from the draining container itself, read back with the same inspect that already checks whether its check is drain-guarded. A healthcheck is baked in at creation, so the spec being deployed describes the containers being started, never the ones being drained. Budgeting from the spec would leave the failure intact for one more deploy after any change to the probe timing — including a change to Onebox's own default, which no operator asked for and which would otherwise strand every replica of the first deploy after an upgrade. Alongside that, the generated healthcheck stops leaving anything to the runtime's defaults. Interval, retries and start period are all written down, so what Onebox reasons about and what the container does cannot drift apart. An unset interval becomes 5s rather than the runtime's 30s: a probe every five seconds costs twelve requests a minute per container and lets a drained one leave rotation in fifteen, which keeps a rolling deploy from being dominated by waiting for the flip. Writing down a fast interval is only safe with a grace period to match. Before, the runtime's 30s interval gave a booting container roughly a minute and a half before a failed probe could count against it; a 5s interval with no start period would call a slow-starting application unhealthy while it is still coming up, and that verdict is visible to dependency conditions, restart watchers and alerting long before the rollout would notice. The start period is therefore written too, at 30s, which is the delay the first probe already had. The runtime leaves it at the first success, so a fast application pays none of it. The readiness budget stretches to cover one full flip cycle when the probe timing needs it. 120s is a figure chosen for ordinary timings, and with `interval: 3m` the newcomer's first probe lands after the budget expires — the rollout would remove a container that was never given the chance to answer. An authored `within` is still taken at its word. Retries gains an upper bound. It multiplies the probe interval to give the budget, and a count large enough to overflow that arithmetic yields a negative budget: one that expires immediately, which is the original failure reached by another route. Validating that retries × interval fits the budget is deliberately not added. The budget is (retries + 2) × interval against a flip of at most (retries + 1) × interval, so it covers the flip by construction. Every generated runtime that declares a healthcheck changes, because it now carries three keys it previously left to the runtime. Frozen corpus verdicts are regenerated accordingly. Closes #109
1 participant
Closes #109.
A rolling deploy marks a container unhealthy, waits for the runtime to notice, then stops it — so the proxy has stopped routing before the container goes away. That wait was budgeted as
retries × pollEvery, wherepollEveryis how often Onebox runsdocker inspect. The flip it waits for happens atretries × the interval the container's own healthcheck runs at.Different jobs: one is a local query, the other a probe inside the container. They agreed only when
health.intervalwas authored. With the shorthandhealth: /pathno interval was written into the generated healthcheck at all, so Docker applied its own 30s default while Onebox budgeted against its 2s poll cadence — a flip needing 90s against a 10s budget. It timed out on every replica of every deploy and stopped each container while the proxy might still be routing to it, which is exactly what the budget exists to prevent.The budget now comes from the container being drained
Not from the spec. A healthcheck is baked in at creation, so the spec describes the containers being started, never the ones being drained.
This distinction is the whole fix. Budgeting from the spec is correct going forward but leaves the failure intact for one more deploy after any change to the probe timing — including a change to Onebox's own default, which no operator asked for and which would otherwise strand every replica of the first deploy after upgrading. It is read back with the same
docker inspectthat already checks whether the check is drain-guarded, widened from.Config.Healthcheck.Testto.Config.Healthcheck, with Docker's own defaults for omitted fields.TestDrainBudgetCoversAContainerBakedBeforeTheChangepins that case: a container whose baked healthcheck has no interval or retries, exactly what Onebox emitted before this change.Nothing is left to the runtime's defaults
interval,retries, andstart_periodare all written into the generated healthcheck now, so what Onebox reasons about and what the container does cannot drift apart. An unset interval becomes 5s rather than the runtime's 30s: twelve probes a minute per container, and a drained one leaves rotation in fifteen seconds, which keeps a rolling deploy from being dominated by waiting for the flip.Writing down a fast interval is only safe with a grace period to match. Before, the runtime's 30s interval gave a booting container roughly 90s before a failed probe could count against it; 5s with no start period would call a slow-starting application unhealthy while it is still coming up — a verdict visible to dependency conditions, restart watchers, and alerting long before the rollout would notice.
start_periodis therefore written too, at 30s, which is the delay the first probe already had. The runtime leaves the start period at first success, so a fast application pays none of it.Two adjacent holes this exposed
withindefaults to 120s, chosen for ordinary probe timings. Withinterval: 3mthe newcomer's first probe lands after it expires, so the rollout wouldrm -fa container that was never given the chance to answer. The default now stretches to cover one full flip cycle; an authoredwithinis still taken at its word.retrieshad no upper bound. It multiplies the probe interval to give the budget, and a count large enough to overflow that arithmetic yields a negative budget — one that expires immediately, which is the original failure reached by another route. Bounded at 1000, orders of magnitude above any real healthcheck.Not added, deliberately
The issue suggested validating that
retries × intervalfits the drain budget. The budget is(retries + 2) × intervalagainst a flip of at most(retries + 1) × interval, so it covers the flip by construction — there is no longer a configuration that can fail this way to warn about.Testing
A virtual clock that advances only when the engine sleeps, plus a fake that models Docker properly: a drained container keeps reporting
healthyuntil the flip is actually due. The previous fake flipped the instant it was drained, which is why a budget too short for the real flip never failed a test.Both drain tests use worst-case phase alignment — a real probe cycle is not aligned to the moment the drain file appears, so a budget of exactly
retries × intervalwould pass a best-case test and still strand containers in the field. Both also assert the drain wait actually ran, since a rollout that decides the check cannot be drain-guarded skips the wait entirely and would pass vacuously.just check,just lint— passgo test -race ./...— pass, 1619 testsONEBOX_E2E=1 go test ./e2e/with Docker — pass (296s)interval: 5s,retries: 3,start_period: 30sCompatibility
Every generated runtime that declares a healthcheck changes, because it now carries three keys it previously left to the runtime. Frozen corpus verdicts are regenerated accordingly. Behaviour for anyone who already authored
interval,retries, andstart_periodis unchanged.🤖 Generated with Claude Code